r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Seem reasonable, did you verify this fixed the bug locally? if so r=me

I have not, it would be kinda annoying to, so I might as well go all the way and just write a run-make test for this.

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@BoxyUwU run-make test added, and CI is passing (I don't have bors perms)

@bors r+ rollup

📌 Commit d1d0b9b has been approved by BoxyUwU

It is now in the queue for this repository.

@bors r-, the tests failed in #138020 (comment)

@bors r-

well that seems incorrect, run-make tests should have access to a full sysroot, and it got past initial CI...

The test might need to be restricted to only targets with std, the failure was testing wasm.

ah, right, of course, CI tests less platforms.

i'll just add a cfg.

It would be preferable to use a //@ directive so we don't even spend the time building an empty test. But surprisingly I'm not seeing something like needs-std or ignore-no-std, @jieyouxu what's the best practice here?

Oh I didn't look at this too closely. Instead of run-make, can this test be ui-fulldeps?

Otherwise you need to carefully match up the rustc invoked by the underlying cargo's sysroot to usr rustc_private, and frankly I don't think it's worth the effort. (For this I wod even say Works Locally is good enough.)

@jieyouxu as far as i can tell, ui-fulldeps tests are built directly with rustc, and we specifically need the environment variables that cargo sets.

will run-make cargo actually default to the global sysroot, and not the one built by bootstrap? if so, that seems like a major hazard, since plenty of other run-make tests use cargo, and some even use rustc_private.

is passing CI good enough for "works locally"? if so i could just rip out the test now that we know it works.

I think its fine to r+ this if you just remove the test since we know it works.

@jieyouxu as far as i can tell, ui-fulldeps tests are built directly with rustc, and we specifically need the environment variables that cargo sets.

Right.

will run-make cargo actually default to the global sysroot, and not the one built by bootstrap? if so, that seems like a major hazard, since plenty of other run-make tests use cargo, and some even use rustc_private.

It depends. On --stage=1 or higher, the cargo() from run_make_support will use the rustc from the corresponding staged sysroot. But on --stage=0, it can depend on what initial rustc/cargo you specify.

IOW, the cargo() from run_make_support is used to ensure something can be built. You should not use it to try to build something that depends on artifacts from the bootstrap-built sysroots. The ones that use cargo() typically uses -Zbuild-std=core or otherwise do not depend on bootstrap-built compiler artifacts.

I.e. you use it as a wrapper to test the underlying rustc's behavior but you cannot depend on the rustc-under-test's libraries.

is passing CI good enough for "works locally"? if so i could just rip out the test now that we know it works.

Yes. I think for the purposes of the changes here, building at all is good enough.

@BoxyUwU test un-pushed.

@bors r+ rollup

📌 Commit 38b364b has been approved by BoxyUwU

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.